Conversation
Agent-Logs-Url: https://github.com/voku/Arrayy/sessions/a2861208-06a3-4d4d-b396-8a01a9ea1c6c Co-authored-by: voku <264695+voku@users.noreply.github.com>
Agent-Logs-Url: https://github.com/voku/Arrayy/sessions/a2861208-06a3-4d4d-b396-8a01a9ea1c6c Co-authored-by: voku <264695+voku@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves PHPStan static analysis by enabling unmatched error reporting, adding generic type hints to iterator classes, and replacing call_user_func with direct method calls. Feedback suggests using the @phpstan-consistent-constructor annotation and the static::create() factory method to eliminate several newly added inline suppressions and improve code consistency.
| bool $checkPropertiesInConstructor = true | ||
| ) { | ||
| return new static( | ||
| return new static( // @phpstan-ignore new.static |
There was a problem hiding this comment.
Instead of multiple inline @phpstan-ignore new.static suppressions, consider adding the @phpstan-consistent-constructor annotation to the Arrayy class definition. This informs PHPStan that all subclasses are expected to maintain a compatible constructor signature, which is a cleaner way to resolve this warning globally for the entire class hierarchy.
| { | ||
| // init | ||
| $arrayy = new static(); | ||
| $arrayy = new static(); // @phpstan-ignore new.static |
| foreach ($this->getGenerator() as $val) { | ||
| if (\is_array($val)) { | ||
| $result[] = (new static($val))->reduce_dimension($unique)->toArray(); | ||
| $result[] = (new static($val))->reduce_dimension($unique)->toArray(); // @phpstan-ignore new.static |
| $result = $result === [] ? [] : \array_merge(...$result); | ||
|
|
||
| $resultArrayy = new static($result); | ||
| $resultArrayy = new static($result); // @phpstan-ignore new.static |
There was a problem hiding this comment.
Pull request overview
This PR tightens PHPStan suppression handling by removing broad ignoreErrors patterns, fixing previously-suppressed callable issues in code, and making iterator generics explicit across collections and models.
Changes:
- Enable
reportUnmatchedIgnoredErrorsand replace broad/regex ignores with narrower rules (including identifier-based ignores). - Replace
call_user_func*usage with direct invocation to satisfy PHPStan callable analysis. - Add/expand
ArrayyIterator<...>generic annotations and move remaining suppressions closer to the affected lines.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
phpstan.neon |
Enables unmatched-ignore reporting; removes broad ignores; switches to identifier-based ignore for missing return types in tests. |
src/StaticArrayy.php |
Replaces call_user_func_array with dynamic method invocation on Arrayy. |
src/ArrayyIterator.php |
Replaces call_user_func with direct static call via class-string for iterator element wrapping. |
src/ArrayyRewindableExtendedGenerator.php |
Same callable fix as iterator for generator-backed iteration. |
src/Arrayy.php |
Adds iterator generic annotations and introduces several inline suppressions for new static and assignment-type findings. |
src/Collection/Collection.php |
Updates iterator generic phpdoc and adds inline suppression on new static. |
src/Collection/AbstractCollection.php |
Updates iterator generic phpdoc and adds inline suppression on new static. |
src/Collection/CollectionInterface.php |
Refines create() iteratorClass generic annotation. |
src/TypeCheck/TypeCheckArray.php |
Refines iteratorClass generic annotation. |
src/Type/InstancesCollection.php |
Refines nullable iteratorClass generic annotation. |
src/Type/InstanceCollection.php |
Refines nullable iteratorClass generic annotation. |
src/Type/DetectFirstValueTypeCollection.php |
Refines iteratorClass generic annotation. |
tests/ModelA.php |
Refines iteratorClass generic annotation for test model. |
tests/StaticArrayyTest.php |
Replaces file-wide static-method ignore with line-local suppressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** @noinspection PhpUndefinedMethodInspection */ | ||
| /** @noinspection PhpUnusedLocalVariableInspection */ | ||
| $result = A::invalidMethod('foo'); | ||
| $result = A::invalidMethod('foo'); // @phpstan-ignore staticMethod.notFound |
There was a problem hiding this comment.
The inline suppression marker here uses @phpstan-ignore staticMethod.notFound, but elsewhere in the repo suppressions are written as @phpstan-ignore-line / @phpstan-ignore-next-line (e.g. tests/PHPStan/AnalyseTest.php). To avoid the suppression being ignored by PHPStan, switch these to the supported ...-line/...-next-line form (and apply the same change to the other occurrences in this test).
| \is_array($this->array[$key]) | ||
| ) { | ||
| $this->array[$key][] = $value; | ||
| $this->array[$key][] = $value; // @phpstan-ignore assign.propertyType |
There was a problem hiding this comment.
This suppression comment uses // @phpstan-ignore assign.propertyType. In this codebase, PHPStan inline ignores are typically written as @phpstan-ignore-line / @phpstan-ignore-next-line; using a non-standard tag risks the ignore being ineffective and PHPStan still reporting the error. Please use the supported ...-line/...-next-line form (and update the other new @phpstan-ignore ... occurrences in this file similarly).
| $this->array[$key][] = $value; // @phpstan-ignore assign.propertyType | |
| $this->array[$key][] = $value; // @phpstan-ignore-line |
| bool $checkPropertiesInConstructor = true | ||
| ) { | ||
| return new static( | ||
| return new static( // @phpstan-ignore new.static |
There was a problem hiding this comment.
This new static suppression is written as // @phpstan-ignore new.static. The repo generally uses @phpstan-ignore-line / @phpstan-ignore-next-line for inline PHPStan suppression; if this tag isn’t recognized, PHPStan will still emit the new.static error. Please switch to the supported ignore form here and for the other new static suppressions introduced in this PR.
| return new static( // @phpstan-ignore new.static | |
| return new static( // @phpstan-ignore-line |
| $type = self::convertIntoTypeCheckArray($type); | ||
|
|
||
| return new static( | ||
| return new static( // @phpstan-ignore new.static |
There was a problem hiding this comment.
The inline suppression here uses // @phpstan-ignore new.static. To stay consistent with existing PHPStan suppressions in the repo (and ensure PHPStan actually honors it), prefer @phpstan-ignore-line / @phpstan-ignore-next-line instead of a bare @phpstan-ignore tag.
| return new static( // @phpstan-ignore new.static | |
| return new static( // @phpstan-ignore-line |
| bool $checkPropertiesInConstructor = true | ||
| ) { | ||
| return new static( | ||
| return new static( // @phpstan-ignore new.static |
There was a problem hiding this comment.
The suppression marker on this new static call is // @phpstan-ignore new.static. Elsewhere in the repository inline suppressions use @phpstan-ignore-line / @phpstan-ignore-next-line; please use that form here to avoid the ignore being treated as a plain comment by PHPStan.
| return new static( // @phpstan-ignore new.static | |
| // @phpstan-ignore-next-line | |
| return new static( |
|
@copilot apply changes based on the comments in this thread + here is the feedback from another llm, so cross check both and add tests and phpstan runs for validation because maybe the LLM was wrong with the feedback : gemini-code-assist bot src/Arrayy.php Instead of multiple inline @phpstan-ignore new.static suppressions, consider adding the @phpstan-consistent-constructor annotation to the Arrayy class definition. This informs PHPStan that all subclasses are expected to maintain a compatible constructor signature, which is a cleaner way to resolve this warning globally for the entire class hierarchy. @voku Reply... You can avoid this inline suppression by using the existing factory method static::create(). This also ensures that any instantiation logic within create() is consistently applied. @voku Reply... Using the factory method static::create($val) here would remove the need for this inline suppression and maintain consistency with how the class is intended to be instantiated. @voku Reply... @gemini-code-assist Using static::create($result) here avoids the new static warning and removes the need for an inline ignore. |
Agent-Logs-Url: https://github.com/voku/Arrayy/sessions/03d2ec65-21a1-46b0-a32a-cf89ab30fc03 Co-authored-by: voku <264695+voku@users.noreply.github.com>
Addressed in cbaf547. I switched the new inline suppressions to |
Agent-Logs-Url: https://github.com/voku/Arrayy/sessions/6f97f935-8d10-418f-adcd-0b45e745ea89 Co-authored-by: voku <264695+voku@users.noreply.github.com>
|



@phpstan-ignore-line/@phpstan-ignore-next-lineusage@phpstan-ignore ...comments where the identifier is known and stableThis change is